Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken CI and improve it #57

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Fix broken CI and improve it #57

merged 6 commits into from
Aug 22, 2024

Conversation

faern
Copy link
Member

@faern faern commented Aug 22, 2024

The initiative for this comes from the current CI being broken. See https://github.com/mullvad/system-configuration-rs/actions/runs/10504169497/job/29099041677

This is sort of a follow up to all the CI improvements we did in pfctl-rs and nftnl-rs a few months back. This repository also needed some love when it comes to CI. Good we have great, pretty much just copy-paste workflows from other crates.

I also added the permissions: {} thing. Something I just today started working on in our main repo also: mullvad/mullvadvpn-app#6660


This change is Reviewable

New linting job almost verbatim copied from pfctl-rs
We have decided in another library project that it does not make much
sense to audit library-only crates. It creates more needless churn than
it helps.
Copied from nftnl-rs.
Also checks minimal versions now.
Also checks that docs build cleanly
@faern faern requested a review from dlon August 22, 2024 08:34
@faern faern mentioned this pull request Aug 22, 2024
@faern faern changed the title Improve CI Fix broken CI and improve it Aug 22, 2024
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern)


.github/workflows/build-and-test.yml line 24 at r1 (raw file):

        # Keep MSRV in sync with rust-version in Cargo.toml
        rust: [stable, beta, nightly, 1.64.0]
    runs-on: macos-latest

We could probably throw *-apple-ios targets into the matrix here, or at least aarch64-apple-ios.


.github/workflows/linting.yml line 14 at r1 (raw file):

jobs:
  clippy-linting:
    runs-on: macos-latest

We could probably throw *-apple-ios targets into the matrix here, or at least aarch64-apple-ios.

Copy link
Member Author

@faern faern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)


.github/workflows/build-and-test.yml line 24 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We could probably throw *-apple-ios targets into the matrix here, or at least aarch64-apple-ios.

This PR is still strictly an upgrade even if we don't do that. So unless you see a lot of benefit in doing so, I'd prefer to get this merged without that, and then anyone can tinker with improving it in that way later if they feel like it. I don't have the time to work too much on this right now. I mostly want to just fix the CI.

My understanding is that your feedback is a suggestion for an unrelated improvement, rather than an issue with the current state of the PR?

@faern faern merged commit e56dcf9 into main Aug 22, 2024
8 checks passed
@faern faern deleted the improve-ci branch August 22, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants